-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement muldiv assign for Ratio #364
base: master
Are you sure you want to change the base?
Conversation
#[inline(always)] | ||
fn $muldivassign_fun(&mut self, rhs: Quantity<DimensionOne, Ur, V>) { | ||
self.value $muldivassign_op rhs.value; | ||
// change_base is needed for autoconvert! version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoconvert!
and not_autoconvert!
versions are needed. Scroll up a bit in the file and see $MulDivTrait
for examples. The autoconvert!
versions should use change_base::<Dr, Ul, Ur, V>(&rhs.value)
. The not_autoconvert!
parameter should only have one U
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I had hoped to test everything that needs to be implemented. But I guess that the not_autoconvert!
version is only needed when the autoconvert
feature is disabled, while the tests always run with the autoconvert
feature enabled. So the tests simply do not test the not_autoconvert!
versions at all. Is that right?
Ifs so, do you have any ideas about how to test the not_autoconvert!
versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full test suite has test runs with and without autoconvert
. Most of the tests do operations where both quantities have the same base units and run for both. Then there are some that are only run when autoconvert
is enabled and ensure that operations with different base units work as expected.
Test cases:
uom/.github/workflows/ci-full-test-suite.yml
Lines 41 to 51 in f8caf07
- name: Test si | |
uses: actions-rs/cargo@v1 | |
with: | |
command: test | |
args: --verbose --no-default-features --features "f32 si" | |
- name: Test all non-storage type features | |
uses: actions-rs/cargo@v1 | |
with: | |
command: test | |
args: --verbose --no-default-features --features "autoconvert f32 si use_serde" |
Tests:
Lines 54 to 70 in f8caf07
#[cfg(feature = "autoconvert")] | |
quickcheck! { | |
#[allow(trivial_casts)] | |
fn add(l: A<V>, r: A<V>) -> TestResult { | |
let km: V = <kilometer as crate::Conversion<V>>::coefficient().value(); | |
let f = (&*l + &*r) / &km; | |
let i = (&*l / &km) + (&*r / &km); | |
if !Test::approx_eq(&i, &f) { | |
return TestResult::discard(); | |
} | |
TestResult::from_bool( | |
Test::approx_eq(&k::Length::new::<meter>(&*l + &*r), | |
&(k::Length::new::<meter>((*l).clone()) | |
+ f::Length::new::<meter>((*r).clone())))) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR
Apoogies for being dense and verbose. It seems that I still don't understand what autoconvert
does.
My understanding is that the autoconvert
feature enables binary operations to act on different Unit
s within the same Quantity
. In other words, without autoconvert
operations like something in metres + something in kilometres
should not work.
The behaviour of the code appears to contradict my understanding.
Gory details
I've added an ac
switch to the test!
macro, so that
test!(ac ... etc) // runs only when autoconvert feature enabled
test!( ... etc) // runs always
In the last commit I've pushed (entitiled: REVERT ME: run 'ac' tests even when autoconvert not enabled
) I have commented out the feature gate, so that the ac
tests run even if autoconvert
is not enabled. At this point, I would expect the six tests labelled with ac
in
... which for some reason doesn't get expanded in the GH interface, so I'll paste the relevant lines by hand:
test!(ac add m(1000.) + km(2.), m(3000.)); // 1
test!(ac sub m(8000.) - km(2.), m(6000.)); // 1
test!( add m( 1.) + m(2.), m(3.)); // 2
test!( sub m( 8.) - m(2.), m(6.)); // 2
test!(ac add_assign [ m(1000.) ] += km(2.), km(3.)); // 3
test!(ac sub_assign [ m(8000.) ] -= km(2.), km(6.)); // 3
test!( add_assign [ m( 1.) ] += m(2.), m(3.)); // 4
test!( sub_assign [ m( 8.) ] -= m(2.), m(6.)); // 4
test!(ac mul m(4000.) * km(2.), km2(8.)); // 5
test!(ac div m(6000.) / km(2.), ratio(3.)); // 5
... so, I expect those six test marked with ac
(just inside test!(
) either to FAIL or to FAIL TO COMPILE, if the autoconvert
feature is not enabled. I expect this, because they all use different Unit
s (belonging to the same Quantity
) on their LHS/RHS. Specifically ac add
uses m
on the LHS and km
on the RHS: these are both Length
s, but have different Unit
s. According to my understanding this should only work if autoconvert
is enabled.
And yet, they not only compile but they also run and pass, when autoconvert
is disabled:
cargo test impl --no-default-features --features "f32 si" ─╯
Compiling uom v0.33.0 (/home/jacek/rust/uom)
Finished test [unoptimized + debuginfo] target(s) in 3.92s
Running unittests src/lib.rs (target/debug/deps/uom-2f2c30298b0ddf3b)
running 28 tests
test tests::impl_ops::vv::add ... ok
test tests::impl_ops::vv::add_assign ... ok
test tests::impl_ops::vv::div_assign_bare ... ok
test tests::impl_ops::vv::div ... ok
test tests::impl_ops::vv::add_assign_autoconvert ... ok
test tests::impl_ops::vv::div_assign_ratio_p ... ok
test tests::impl_ops::vv::add_autoconvert ... ok
test tests::impl_ops::vv::div_assign_ratio ... ok
test tests::impl_ops::vv::div_autoconvert ... ok
test tests::impl_ops::vv::div_bare_left ... ok
test tests::impl_ops::vv::div_bare_right ... ok
test tests::impl_ops::vv::div_ratio_left ... ok
test tests::impl_ops::vv::div_ratio_right ... ok
test tests::impl_ops::vv::div_ratio_right_p ... ok
test tests::impl_ops::vv::mul ... ok
test tests::impl_ops::vv::mul_assign_bare ... ok
test tests::impl_ops::vv::mul_assign_ratio ... ok
test tests::impl_ops::vv::mul_assign_ratio_p ... ok
test tests::impl_ops::vv::mul_autoconvert ... ok
test tests::impl_ops::vv::mul_bare_left ... ok
test tests::impl_ops::vv::mul_bare_right ... ok
test tests::impl_ops::vv::mul_ratio_left ... ok
test tests::impl_ops::vv::mul_ratio_right ... ok
test tests::impl_ops::vv::mul_ratio_right_p ... ok
test tests::impl_ops::vv::sub ... ok
test tests::impl_ops::vv::sub_assign ... ok
test tests::impl_ops::vv::sub_assign_autoconvert ... ok
test tests::impl_ops::vv::sub_autoconvert ... ok
test result: ok. 28 passed; 0 failed; 0 ignored; 0 measured; 174 filtered out; finished in 0.00s
Points to note:
--no-default-features
--features "f32 si"
does NOT includeautoconvert
- The 6 tests whose names end in
_autoconvert
run and pass.
Where is my error / misunderstanding / brain fart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the autoconvert feature enables binary operations to act on different Units within the same Quantity. In other words, without autoconvert operations like something in meters + something in kilometers should not work.
autoconvert
affects how quantities with different base units work. A uom
Quantity
doesn't store the original unit (e.g. meter, kilometer, ...) that was used. Instead the value is normalized into a base unit. Meter by default in the case of length. When talking about operations between quantities we can be more explicit by saying a length (the D
generic parameter of quantity) stored as meters (the U
generic parameter of quantity) inf32
(the V
generic parameter of quantity).
let m = Length::new::<meter>(50.0); // Internally stored as 50.0 meters.
let km = Length::new::<kilometer>(50.0); // Internally stored as 50_000.0 meters.
You're never adding units together, always quantities. autoconvert
affects how operations are defined such that when autoconvert
is not enabled you can only operate on quantities with the same base units. e.g. length stored as meters can only operate with length stored as meters, but not length stored as kilometers. When autoconvert
is enabled this limitation is lifted.
Compare https://github.com/iliekturtles/uom/blob/master/examples/si.rs and https://github.com/iliekturtles/uom/blob/master/examples/base.rs. si.rs
will work regardless of the autoconvert
setting because the same base units are used for all quantities. base.rs
requires autoconvert
to be enabled because a new set of base units (using centimeter, gram, second) is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience and your clear explanations. Your description is consistent with my initial understanding of autoconvert
. I'm not sure why, in the process of working on this, I convinced myself that it was something else. Apologies.
I have corrected my confusion, rebased, and force-pushed two commits:
Add concise tests summarizing which binops work
Implement MulDivAssign with Ratio on RHS
The first of these commits summarizes which arithmetic binary operations are implemented, both in the presence and absence of autoconvert
. I have also added some tests for the comparison operators. For now, I'm only testing value operands; the idea is to add ref-ref, value-ref, and ref-value versions of these tests, for use in #307.
The second of these commits is the implementation that I pushed earlier, without any changes. Before this commit, these four tests :
test!( mul_assign_ratio [ m(4.) ] *= one(2.), m(8.)); // c.f. AAA above
test!( div_assign_ratio [ m(6.) ] /= one(2.), m(3.)); // c.f. BBB above
test!(ac mul_assign_ratio [ m(4.) ] *= cgs_one(2.), m(8.)); // c.f. CCC above
test!(ac div_assign_ratio [ m(6.) ] /= cgs_one(2.), m(3.)); // c.f. DDD above
fail to compile (which is why they are commented out in the first commit). After the second commit, all four of these tests pass (which is why they are uncommented in the commit).
The first two of these tests use the si
system that uom
provides out of the box on both the LHS and the RHS. As such they should work both with and without autoconvert
. The second two tests differ from the first two in that they use a set of cgs
units on the RHS; thus they should only work when autoconvert
is enabled. All this is borne out in practice.
Returning to what you said right at the top of this conversation:
autoconvert!
andnot_autoconvert!
versions are needed.
I was hoping that these tests would demonstrate that this is the case. Instead, they seem to be indicating that the single implementation I provided, covers both cases.
So ...
- Have I misunderstood something again?
- If not, why are the two versions needed? Perhaps because, even though the single implementation is a correct implementation covering both cases, it is unnecessarily inefficient in (at least) one of the cases?
I'll try to implement both versions when I find some more time, but I just wanted to check in and make sure that I'm getting back on the right path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm misunderstanding, but it seems to me that change_base
applies a conversion factor which is appropriate for conversion between corresponding quantities in the two different sets of units, but in this case the RHS is Ratio
while the LHS is anything but Ratio
. In my specific test, this results in applying the conversion factor of 100 which converts between cm
and m
, but this is inappropriate, because the RHS is Ratio
.
Also, is it possible to have a 'base unit' for Ratio
which differs from 1? I see how ISQ
can create sets of units with arbitrary base units for the 7 fundamental quantities, but I don't see how Ratio
's base unit could be anything other than 1.
If the above points are approximately correct, then it would seem that separate autoconvert!
and not_autoconvert!
implementations are not necessary ... just like in the Quantity */= f32
case.
23f450f
to
eef466b
Compare
eef466b
to
28699c6
Compare
As discussed here